Skip to content

refactor(all): add middy-like types to commons #1225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Jan 11, 2023

Description of your changes

Following the discussion in the linked issue (#1068), as well as other issues (#1080, #1081), customers who are using Powertools and bundling their TypeScript project with tsc, but that were not using any of the Middy-compatible middleware were having compile issues due to the dependency being missing.

We have investigated several workarounds:

The first option was a no-go as it would have introduced an unnecessary dependency for those users having the issue. The second one would have constituted a breaking change, requiring a major release.

The last option (credits to @saragerion for proposing it) allows us to maintain backwards compatibility while solving the issue described above.

The solution that this PR proposes (again, credits to @saragerion), introduces some of the types previously imported from @middy/core into @aws-lambda-powertools/commons. By bringing these types into the project, we can remove the dependency to @middy/core from our codebase and allow customers to bundle using tsc as well as esbuild.

Note
The @middy/core dependency was used only for its types. Powertools still supports Middy middleware, and customers who want to use them still need to opt-in by adding the dependency to their Lambda functions.

How to verify this change

See existing unit tests below this PR and successful integration test run: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3894429621

Related issues, RFCs

Issue number: #1068

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi self-assigned this Jan 11, 2023
@dreamorosi dreamorosi linked an issue Jan 11, 2023 that may be closed by this pull request
@pull-request-size pull-request-size bot added the size/M PR between 30-99 LOC label Jan 11, 2023
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jan 11, 2023
@dreamorosi dreamorosi force-pushed the 1068-bug-unable-to-build-with-tsc-when-not-using-middycore branch from ef61d0c to 729da39 Compare January 11, 2023 15:40
@obiwabrakenobi
Copy link

Its does not feel correct to copy the code form another repository as major updates need to manually transferred and incompatibilities need always to be tested. But for solving the tsc issue it should be fine 👍

Thanks for the effort

@dreamorosi
Copy link
Contributor Author

Hey @obiwabrakenobi, this is a hack made to fix the tsc issue while staying backwards compatible with previous v1.x versions of Powertools.

Once we start discussing Powertools v2 we'll likely revisit this topic with the freedom of being able to make a breaking change on our side, but for now the tsc issue was preventing adoption.

In the meanwhile we'll keep an eye on the upstream repo (which we are already doing).

@dreamorosi dreamorosi merged commit c78e5d3 into main Jan 13, 2023
@dreamorosi dreamorosi deleted the 1068-bug-unable-to-build-with-tsc-when-not-using-middycore branch January 13, 2023 09:53
@saragerion
Copy link
Contributor

saragerion commented Jan 16, 2023

Hi @obiwabrakenobi completely agree with you that this is not the best solution. As Andrea already mentioned, definitely something we want to improve in v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature PRs that introduce new features or minor changes size/M PR between 30-99 LOC
Projects
None yet
3 participants